Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report mods in ParseCallbacks::new_item_found #3145

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

adetaylor
Copy link
Contributor

Report mods in callbacks, and item parentage.

This makes two complementary improvements to the ParseCallbacks.
The first is that Mods are now announced, as a new type of
DiscoveredItem. The second is that the parentage of each item is
announced. The parent of an item is often a mod (i.e. a
C++ namespace) but not necessarily - it might be a struct within
a struct, or similar.

The reported information here is dependent on two pre-existing
bindgen options:

  • whether to report C++ namespaces at all
  • whether to report inline namespaces conservatively.

For that reason, the test suite gains two new tests.

Builds on top of #3141.

This extends the existing discovery callback mechanism to report on functions
and methods. At this stage, we don't say much about them, in order to be
consistent with other discovery callbacks. Subsequent PRs will add
extra callbacks to provide information especially about methods
(virtualness, C++ visibility, etc.) Please request changes if you think
that sort of information should arrive in these callbacks.

Because methods are a fundamentally C++ thing, this splits the
current ParseCallbacks test to cover both a .h and a .hpp header.

Part of google/autocxx#124
No functional change - just deduplicating the logic which calls this callback,
which will make it easier to make further changes in future.

Part of google/autocxx#124
This adds more information to ParseCallbacks which indicates the location in
the original source code at which a given item was found.

This has proven to be useful in downstream code generators in providing
diagnostics to explain why a given item can't be represented in Rust. (There
are lots of reasons why this might not be the case - autocxx has around 100
which can be found here -
https://github.com/google/autocxx/blob/d85eac76c9b3089d0d86249e857ff0e8c36b988f/engine/src/conversion/convert_error.rs#L39
- but irrespective of the specific reasons, it's useful to be able to point to
the original location when emitting diagnostics).

Should we make this a new callback or include this information within
the existing callback?

Pros of making it a new callback:
* No compatibility breakage.

Pros of including it in this existing callback:
* No need to specify and test a policy about whether such callbacks
  always happen together, or may arrive individually
* Easier for recipients (including bindgen's own test suite) to
  keep track of the source code location received.
* Because we add new items to the DiscoveryItem enum anyway,
  we seem to have accepted it's OK to break compatibility in this
  callback (for now at least).

Therefore I'm adding it as a parameter to the existing callback. If it's
deemed acceptable to break compatibility in this way, I will follow the
same thought process for some other changes too.

Part of google/autocxx#124.
This makes two complementary improvements to the ParseCallbacks.
The first is that Mods are now announced, as a new type of
DiscoveredItem. The second is that the parentage of each item is
announced. The parent of an item is often a mod (i.e. a
C++ namespace) but not necessarily - it might be a struct within
a struct, or similar.

The reported information here is dependent on two pre-existing
bindgen options:
* whether to report C++ namespaces at all
* whether to report inline namespaces conservatively.

For that reason, the test suite gains two new tests.

Part of google/autocxx#124
@adetaylor adetaylor mentioned this pull request Feb 21, 2025
8 tasks
@adetaylor adetaylor marked this pull request as ready for review February 21, 2025 14:27
adetaylor added a commit to adetaylor/rust-bindgen that referenced this pull request Feb 21, 2025
This change reports extra C++ information about items:

* Whether methods are virtual or pure virtual or neither
* Whether a method is a "special member", e.g. a move constructor
* Whether a method is defaulted or deleted
* C++ visibility (for structs, enums, unions and methods)

It builds on top of rust-lang#3145.

This PR is not yet ready for merge, since we need to merge rust-lang#3139
and then enhance the tests to cover those cases too.

Part of google/autocxx#124
adetaylor added a commit to adetaylor/rust-bindgen that referenced this pull request Feb 21, 2025
This change reports extra C++ information about items:

* Whether methods are virtual or pure virtual or neither
* Whether a method is a "special member", e.g. a move constructor
* Whether a method is defaulted or deleted
* C++ visibility (for structs, enums, unions and methods)

It builds on top of rust-lang#3145.

A follow up PR should enhance the tests once rust-lang#3139 is merged.

Part of google/autocxx#124
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant